Skip to content

feat: [RPT] GZIP encoding#779

Open
CharlesDuboisSAP wants to merge 5 commits intomainfrom
gzip
Open

feat: [RPT] GZIP encoding#779
CharlesDuboisSAP wants to merge 5 commits intomainfrom
gzip

Conversation

@CharlesDuboisSAP
Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP commented Mar 4, 2026 •
edited
Loading

Context

AI/ai-sdk-java-backlog#363.

Smaller payload on SAP RPT.
Corresponding Cloud SDK PR:

Feature scope:

  • SAP RPT tableCompletion now GZIP compresses the request payload

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Aligned changes with the JavaScript SDK
  • Documentation updated
  • Release notes updated

@CharlesDuboisSAP CharlesDuboisSAP self-assigned this Mar 4, 2026
@CharlesDuboisSAP CharlesDuboisSAP added the please-review Request to review a pull-request label Mar 4, 2026
@CharlesDuboisSAP CharlesDuboisSAP mentioned this pull request Mar 4, 2026
Open
6 tasks
ZhongpinWang
ZhongpinWang reviewed Mar 5, 2026
View reviewed changes
Jonas-Isr
Jonas-Isr reviewed Mar 5, 2026
View reviewed changes
Copy link
Member

@Jonas-Isr Jonas-Isr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good so far (besides the snapshot in the pomp course). But should we not test this? At least the happy path?

@CharlesDuboisSAP
Copy link
Contributor Author

CharlesDuboisSAP commented Mar 6, 2026 •
edited
Loading

Code looks good so far (besides the snapshot in the pomp course). But should we not test this? At least the happy path?

It's already tested in RptClientTest. The request payload is also checked. I didn't have to do any changes because wiremock converts it automatically I guess

Jonas-Isr
Jonas-Isr reviewed Mar 10, 2026
View reviewed changes
Copy link
Member

@Jonas-Isr Jonas-Isr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code itself looks good. The thing I am wondering is: IIRC the JS team showed in the demo that they only compress large input, right? And that the user has a choice to disable compression? Is there a reason you did not consider that? And what is your estimate on how breaking it would be to add a choice for the user in the future?

@CharlesDuboisSAP
Merge branch 'main' into gzip
4817415 
# Conflicts:
#	docs/release_notes.md
@CharlesDuboisSAP
cleaner code
17aa537
CharlesDuboisSAP
CharlesDuboisSAP commented Mar 11, 2026
View reviewed changes
foundation-models/sap-rpt/src/main/resources/spec/sap-rpt-1_openapi.json
"enum": ["gzip"]
}
}
],
Copy link
Contributor Author

@CharlesDuboisSAP CharlesDuboisSAP Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clemens Biehl from SAP RPT will add this header in the spec

Jonas-Isr
Jonas-Isr approved these changes Mar 11, 2026
View reviewed changes
Copy link
Member

@Jonas-Isr Jonas-Isr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rpanackal rpanackal rpanackal left review comments

@Jonas-Isr Jonas-Isr Jonas-Isr approved these changes

+1 more reviewer

@ZhongpinWang ZhongpinWang ZhongpinWang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@CharlesDuboisSAP CharlesDuboisSAP

Labels

please-review Request to review a pull-request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@CharlesDuboisSAP @rpanackal @ZhongpinWang @Jonas-Isr